-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(io): Align FileStatus with the Java API. #45
Conversation
crates/paimon/src/io/file_io.rs
Outdated
@@ -94,6 +97,13 @@ impl FileIO { | |||
.into_iter() | |||
.map(|meta| FileStatus { | |||
size: meta.metadata().content_length(), | |||
is_dir: meta.metadata().is_dir(), | |||
last_modification_time: meta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing, I found that the directory's last_modification_time
= 0 for a dir. But for the LocalFileIO
in java it's not.
Is this a bug for opendal Fs
service ? CC @Xuanwo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's related to apache/opendal#4615. Would you like to take a look? It might be complex to resolve entirely, but we can start by fixing it for the fs backend first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, will take a look
crates/paimon/src/io/file_io.rs
Outdated
@@ -155,6 +165,9 @@ impl FileIO { | |||
#[derive(Clone, Debug)] | |||
pub struct FileStatus { | |||
pub size: u64, | |||
pub is_dir: bool, | |||
pub path: String, | |||
pub last_modification_time: i64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about using chrono::DateTime
to make it easier to use. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense, updated.
CC @Xuanwo |
crates/paimon/src/io/file_io.rs
Outdated
@@ -72,6 +74,9 @@ impl FileIO { | |||
|
|||
Ok(FileStatus { | |||
size: meta.content_length(), | |||
is_dir: meta.is_dir(), | |||
last_modification_time: meta.last_modified(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using last_modified
or modified
for short?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -34,7 +36,7 @@ impl FileIO { | |||
/// | |||
/// TODO: Support building Operator from HashMap via options. | |||
pub fn new(_: HashMap<String, String>) -> Result<Self> { | |||
let op = Operator::from_config(MemoryConfig::default()) | |||
let op = Operator::new(Fs::default().root("/")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better not to hard-code fs here, but I think it's a good starting point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I think here should construct the fileIO according to the scheme. I'd like to do it in another pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
close #44